Skip to content

[SC-8445] fix: Apply post_process_fn to individual results in comparison tests#310

Closed
juanmleng wants to merge 1 commit intomainfrom
juan5508/sc-8445/fix-apply-post_process_fn-to-individual-results-in-comparison-tests
Closed

[SC-8445] fix: Apply post_process_fn to individual results in comparison tests#310
juanmleng wants to merge 1 commit intomainfrom
juan5508/sc-8445/fix-apply-post_process_fn-to-individual-results-in-comparison-tests

Conversation

@juanmleng
Copy link
Contributor

@juanmleng juanmleng commented Feb 7, 2025

Internal Notes for Reviewers

When running comparison tests with input_grid or param_grid and a post_process_fn, the post-processing function is applied to the combined results rather than to individual test results. This can lead to an unexpected or unintuitive display of outputs, as post-processing functions are not associated with a specific input_id but are instead applied to the aggregated results.

External Release Notes

@juanmleng juanmleng added bug Something isn't working internal Not to be externalized in the release notes labels Feb 7, 2025
@juanmleng juanmleng self-assigned this Feb 7, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2025

PR Summary

This pull request introduces a new feature to the _run_comparison_test function in the validmind/tests/run.py file. The enhancement allows for an optional post_process_fn parameter, which is a callable function that can be applied to each TestResult after the test is run. This provides additional flexibility for users to modify or analyze test results after they are generated. The changes include:

  • Addition of the post_process_fn parameter to the _run_comparison_test function signature.
  • Modification of the test execution loop to apply the post_process_fn to each result if it is provided.
  • Passing the post_process_fn parameter to the run_test function, ensuring that it is available for further processing.

These changes enhance the test framework by allowing custom post-processing of test results, which can be useful for logging, result transformation, or additional validation steps.

Test Suggestions

  • Test the _run_comparison_test function with a valid post_process_fn to ensure it correctly modifies the TestResult.
  • Verify that _run_comparison_test behaves as expected when post_process_fn is None.
  • Check the integration of post_process_fn with run_test to ensure it is passed and utilized correctly.
  • Test edge cases where post_process_fn might raise exceptions to ensure they are handled gracefully.

@juanmleng juanmleng requested a review from johnwalz97 February 7, 2025 22:07
johnwalz97
johnwalz97 requested changes Feb 11, 2025
View reviewed changes
validmind/tests/run.py
Copy link
Contributor

@johnwalz97 johnwalz97 Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure that I agree with the choice to run post-process functions for each individual result in a comparison test instead of on the final result. Although it does make sense to have the ability to do so. Maybe we should discuss in vm-library channel to see what the team thinks?

Copy link
Contributor

@AnilSorathiya AnilSorathiya Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This interesting point raised by @johnwalz97. Somewhat I agree with him. It would be clean if we keep post-process outside in the hand of users to process using the results/output of a test.

We have rawData available in the output of a test. @juanmleng can we use it in post-proces function to get desirable results?

Copy link
Contributor Author

@juanmleng juanmleng Feb 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnwalz97 raised a very valid point here, and made me think about this a bit more. I totally agree @AnilSorathiya If we add the input_id to the RawData in our tests and leave the user to handle the combined and individual results in the post-processing, we can indeed get to the desired results.

@juanmleng
Copy link
Contributor Author

juanmleng commented Feb 18, 2025

Closing this PR as we've decided that maintaining the current API is the best approach.

Next steps:

  1. Create a new PR to add input IDs to RawData in our tests
  2. This will better support post-processing in comparison tests
  3. Document post-processing and RawData for comparison tests in our how-to guides

The existing RawData + processing_fn architecture already provides the flexibility we need, we just need to leverage it better.

@juanmleng juanmleng closed this Feb 18, 2025
@juanmleng
Copy link
Contributor Author

juanmleng commented Feb 18, 2025

Closing this PR as we've decided that maintaining the current API is the best approach.

Next steps:

  1. Create a new PR to add input IDs to RawData in our tests
  2. This will better support post-processing in comparison tests
  3. Document post-processing and RawData for comparison tests in our how-to guides

The existing RawData + processing_fn architecture already provides the flexibility we need, we just need to leverage it better.

@johnwalz97 johnwalz97 deleted the juan5508/sc-8445/fix-apply-post_process_fn-to-individual-results-in-comparison-tests branch August 20, 2025 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@johnwalz97 johnwalz97 johnwalz97 requested changes

@AnilSorathiya AnilSorathiya Awaiting requested review from AnilSorathiya

@cachafla cachafla Awaiting requested review from cachafla

Assignees

@juanmleng juanmleng

Labels

bug Something isn't working internal Not to be externalized in the release notes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@juanmleng @johnwalz97 @AnilSorathiya